-
Notifications
You must be signed in to change notification settings - Fork 263
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
webadmin: add Account Settings sub-tab #130
Conversation
As we discussed offline, I really don't like this new suggested tab and IMHO it causes a lot of problems and complexity:
|
This is a new proposal - not covered by our previous design. Not sure what were the reasons for dropping this kind of functionality but the area changed significantly in the last 3 years so IMHO changes are justified.
Please take another look at the PR description.
Please take another look at the PR description.
Please provide more details about those issues.
|
@rszwajko I'm still not convinced about the necessarily of this dialog. We are not adding dialogs just for troubleshooting problems on webadmin. The only motivation is if there is a functional requirement for that and I'm not sure if this is the case. Not all functionality supported by REST should be supported by the UI unless it's required. When only In that case, I would recommend dividing the properties into vm portal settings and webadmin settings and also have an option to clean all and add a confirmation dialog with the number of removed properties etc. In case of an error, the following ACTION_TYPE_FAILED_USER_PROFILE_PROPERTY_NOT_EXISTS message appears: The action is set to |
We are adding more and more features that rely on user options so it makes sense to provide some management capabilities.
No change here compared with current situation - user options are outside of permission system so any admin have full access.
We don't have a flag to separate both types. We mark it only with prefix but this is more a convention.
+1
the props are visible all the time in the table so not sure if this is needed.
|
As long as '
Sorting by property name is possible, right? This might help diving between those 2 types.
I think that a confirmation dialog with number of removed properties is needed here since you are about to remove properties for another user, need an option to verify that.
? |
This seems a new requirement. The only permission check currently performed is
Yes. It's already implemented in this version.
+1
No changes here - it's the same in the current code. This the backend validation message:
The |
Why |
The command is correct. The action name is Line 52 in cdd24a4
|
This is a bit confusing....but ok
I guess it will work like on rest but need to verify |
@rszwajko is there a BZ related to this PR? Can this wait for 4.5.1 or should this be included in 4.5.0? |
No specific BZ but it may get included into the umbrella BZ https://bugzilla.redhat.com/2049782
It's definitely 4.5.1 |
missed 4.5.1 as well. |
@rszwajko 1) Are we ok with a super user that doesn't include
I disagree. For non admin users this is blocked since each user can manipulate his own account settings only (via rest and vm portal). So we are ok with that. 2)
I think it worth the effort... |
@sgratch
Please provide more details about |
/ost |
As an example you can have a user with a customized admin role that doesn't include the Line 74 in 0dabcf4
For reproducing that, you can create a customized admin role via administration->configure->roles with system->'manipulate users' action disabled. |
@sgratch |
@rszwajko 2) I think that 'reset settings' reachable from the kebab menu for columns is a bit confusing when reachable from the account settings sub tab, since it's not clear which settings to reset here. But let's leave it as is for now since it is consistent with other tabs and we can always rephrase the label in the future if asked for |
@rszwajko @mwperina
There is no option to manage a role for that since the It seems like the logic for that is implemented here: Lines 43 to 49 in 4655358
I can see that checking if a user profile property can be handled by other user is checked here: Lines 58 to 62 in 4655358
and is applied only for adding/updating a user property but doesn't apply for deleting a user property. So bottom line, this is all part of the user settings feature implementation and I'm not sure what was the original implementation for SSH keys removal and do we want to leave it as is or treat it as a bug before exposing this account settings tab. @mwperina Please review this third issue comment. Do you think it is a bug or remember if in old implementation (prior account settings), admin users could delete other admin user's ssh keys without an option to block it by permissions? |
I think we should change the check to SuperUser, that should be fine enough - just let the SuperUser(s) to be able to delete profiles, that's it |
Before, in order to display a toast notification, the component needed direct access to NotificationPresenterWidget. With this patch this restriction has been removed. The notification handler registers itself in the Frontend which allows public access.
Add a new sub-tab under Administration -> Users -> {user} that allows viewing user settings. The settings are displayed unformated as JSON or plain text depending on the property type. Only remove operation is supported. Main use case is troubleshooting problems related to user settings. With the new view, the admin is able to restore the default configuration by removing incorrect user properties.
3d41dbf
to
8ba58ac
Compare
you mean i.e. via this helper ovirt-engine/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java Line 1165 in c25c3f3
|
@michalskrivanek @sgratch |
@rszwajko yes. |
/ost |
/ost |
Depends on #129
Add a new sub-tab under Administration -> Users -> {user} that allows
viewing user settings. The settings are displayed unformated as JSON
or plain text depending on the property type. Only remove operation is
supported.
Main use case is troubleshooting problems related to user settings.
With the new view, the admin is able to restore the default
configuration by removing incorrect user properties.